-
Notifications
You must be signed in to change notification settings - Fork 49.8k
[Fiber] Child Reconciliation, Refs and Life-Cycles #7707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| deleteInstance(instance : I) : void, | ||
|
|
||
| createTextInstance(text : string) : TI, | ||
| commitTextUpdate(textInstance : TI, oldText : string, newText : string) : void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach would overload the existing createInstance and commitUpdate to accept a string and return a Text node. This might seem like it causes unnecessary branching trouble there, but if you think about it, all other instances tend to be more specific types than "View" or "Element". They're all subclasses so you need to branch on them sooner or later anyway.
There isn't necessarily any need to special case Text components here.
It is needed in the internals for reconciliation purposes since string children are special. However, they're not needed here.
You could argue that since they're already special cased by having their own fiber, it allows early branching instead of branching in the switch statement and then branching again inside the host environment adapter.
0285c01 to
03c3155
Compare
| // This function is not recursive. | ||
| // If the top level item is an array, we treat it as a set of children, | ||
| // not as a fragment. Nested arrays on the other hand will be treated as | ||
| // fragment nodes. Recursion happens at the normal flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this means (yet?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't recurse down nested arrays by itself. It yields to the outer work loop by returning a fragment for nested arrays.
|
Commits here look good. |
|
I think we need to add a Otherwise we can't line up the implicitly keyed Bars with themselves in these two cases: <div>{c ? <Foo /> : null}<Bar /><Bar /></div>Maybe if I store the index, on the Fiber, of the implicit key that each Bar had before collapsing the linked list... |
|
Here's a tough one... Reconciling in place, when the set is already WIP, means that side-effects need to be calculated against the current set but we want to reuse any nodes from the WIP. Edit: The key insight here is that in this scenario any existing deletion is still valid. That is because the decision to treat items that was started to be deleted as unrestorable. There can be new deletions added in this pass though. Moves can be recalculated from the index used in the "current". Insertions/moves have to be rescheduled in the side-effect list. |
03c3155 to
44de89d
Compare
|
@spicyj Some more home work for you. :D |
|
GitHub doesn't email about pushes now? >.> |
c4ffa45 to
83661c3
Compare
|
|
||
| var instanceC = instance; | ||
|
|
||
| expect(instanceC === instanceA).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could write this as expect(instanceC).toBe(instanceA)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can't be cause when it errors that stalls jest trying to print the error presumably. I spent some time trying to figure out why but the debugger doesn't work on it. My best bet is that a message simply gets dropped which stalls the process waiting for a response that never arrives. cc @cpojer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not good. Would you mind opening an issue about this on Jest's bugtracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Update | ||
| const existing = useFiber(current, priority); | ||
| existing.pendingProps = textContent; | ||
| existing.return = returnFiber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like whenever useFiber is used, you also set the return fiber. Any reason this is kept separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure yet if the return should move closer to the iteration branch yet. So should sibling ideally. I'm not yet sure where I need to add things to the return fiber such as adding side-effects.
| prev = createFirstChild(returnFiber, existing, newChildren[i], priority); | ||
| first = prev; | ||
| // Mark all children as deleted and add them to a key map for quick lookups. | ||
| const existingChildren = mapAndDeleteRemainingChildren(returnFiber, oldFiber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a stupid question but I've looked at it for a while and still don't get it—why is it called mapAndDeleteRemainingChildren if it iterates over all the children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not all children. oldFiber is the last fiber that wasn't consumed above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Duh. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use some renames in here at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may just be my brain struggling to read mutating code after avoiding it for a while.
1845b08 to
1b5ed5f
Compare
|
I moved spill-over extra todos to separate issue #7906 that I don't plan on doing in this first PR. |
978b81d to
09bd01c
Compare
424e170 to
727ed0f
Compare
We currently treat nested arrays as a unique namespace from top level children. So that these won't share key namespaces. This adds a new fiber type that will represent the position of a fragment. This is only used for nested arrays. Even if you return an array from a composite component, we don't need this since they share namespace with a single top level component. This still doesn't implement the complete reconciliation algorthim in child fiber. That's coming later.
These nodes rendering into Text nodes in the DOM. There is a special case when a string is a direct child of a host node. In that case, we won't reconcile it as a child fiber. In terms of fibers, it is terminal. However, the host config special cases it. It is kind of unfortunate that we have to special case this kind of child in the HostConfig. It would be nice to unify this with other types of child instances. Text nodes have some weird special cases, but those special cases tend to *vary* by environment. They're not the same special cases so not sure how valuable it is to have a special protocol and special types for it.
When the feature flag is on, we should silence the warning since we're explicitly testing it. This is needed when running unit tests with the flag on.
Dropped the unnecessary use of findDOMNode. Dropped unnecessary arrow functions. Math.random() -> id counter, since this used to be non-deterministic which is not ideal for unit tests. getOriginalKeys() used to rely on implementation details to scan that the internal data structure maintained its structure, however, since that is unobservable we don't need to test the internal data structure itself. We're already testing refs and dom structure which is the only observable effect of the reorder.
This needs to be fixed somehow. The reconciler could know if you are mounting this continuation into the same spot as before and then clone it. However, if the continuation moves, this info is lost. We'd try to unmount it in one place and mount the same fiber in another place.
We'll enable updating of text nodes. To be able to test that we need the text nodes to be mutable. They're currently just strings in the Noop renderer so this makes them an object instead. That exposed a bug in ReactFiberCommitWork for text nodes.
This implements the first step to proper child reconciliation. It doesn't yet track side-effects like insert/move/delete but has the main reconciliation algorithm in place. The goal of this algorithm is to branch early and avoid rechecking those conditions. That leads to some duplications of code. There are three major branches: - Reconciling a single child per type. - Reconciling all children that are in the same slot as before from the beginning. - Adding remaining children to a temporary Map and reconciling them by scanning the map. Even when we use the Map strategy we have to scan the linked list to line up "same slot" positions because React, unlike Inferno, can have implicit keys interleaved with explicit keys.
We use this to track the index slot that this Fiber had at reconciliation time. We will use this for two purposes: 1) We use it to quickly determine if a move is needed. 2) We also use it to model a sparce linked list, since we can have null/false/undefined in our child set and these take up a slot for the implicit key.
We don't need to track side-effects for a parent that has never been mounted before. It will simply inject all its children when it completes.
When we don't have any previous fibers we can short cut this path knowing that we will never have any previous child to compare to. This also ensures that we don't create an empty Map in this case.
When we reconcile children we need to track the deletions that happen so that we can perf side-effects later as a result. The data structure is a linked list where the next pointer uses the nextEffect pointer. However, we need to store this side-effect list for reuse if we end up reusing the progressedChild set. That's why I add a separate first/last pointer into this list so that we can keep it for later.
This allow us to track what kind of side-effect this was even though we only have a single linked list for all side-effects.
First clear any progressed deletions for any case where we start over with the "current" set of children. Once we've performed a new reconciliation we need to add the deletions to the side-effect list (which we know is empty because we just emptied it). For other effects, instead of just adding a fiber to an effect list we need to mark it with an update. Then after completion we add it to the the effect list if it had any effects at all. This means that we lose the opportunity to control if a fiber gets added before or after its children but that was already flawed since we want certain side-effects to happen before others on a global level. Instead, we'll do multiple passes through the effect list.
Instead of passing the full list of children every time to update the host environment, we'll only do inserts/deletes. We loop over all the placement effects first and then later we do the rest.
This shortcut had a bug associated with it. If beginWork on this child returns null, then we don't call completeWork on that fiber. Since removing this short cut adds another time check, we have to add a single unit of time in tests to account for the top level call now taking one more unit. This was also the only recursive call in all of fiber so it's nice to get rid of it to guarantee that a flat stack is possible.
We only use the effect list when we reuse our progressed children. Otherwise it needs to reset to null. In all other branches, other than bailoutOnLowPriority, we revisit the children which recreates this list. We should also not add fibers to their own effect list since it becomes difficult to perform work on self without touching the children too. Nothing else does that neither. Since that means that the root isn't added to an effect list we need to special case the root.
It is not possible for a child to have a higher priority level than we're reconciling at, unless we intentionally want to down-prioritize it.
While we're deleting nodes, we need to call the unmount life-cycle. However, there is a special case that we don't want to keep deleting every host node along the way since deleting the top node is enough.
These happen in the commit phase *before* the setState callback. Unfortunately, we've lost the previous state at this point since we've already mutated the instance. This needs to be fixed somehow.
During the deletion phase we call detachRefs on any deleted refs. During the insertion/update phase we call attachRef on class and host components. Unfortunately, when a ref switches, we are supposed to call all the unmounts before doing any mounts. This means that we have to expact the deletion phase to also include updates in case they need to detach their ref.
This reorganizes the two commit passes so that all host environment mutations happens before any life-cycles. That means that the tree is guaranteed to be in a consistent state at that point so that you can read layout etc. This also lets us to detach all refs in the same pass so that when they get invoked with new instances, that happens after it has been reset.
If we abort work but have some completed, we can bail out if the shouldComponentUpdate returns true. However, then we have a tree that is low priority. When we bailout we currently use the "current" tree in this case. I don't think this is right. I'm not sure why I did that. Similarly, when we complete we use the "current" props if we didn't have pending props to do. However, we should be using the memoized props of that tree if it is a pending work tree. Added a unit test that covers these two cases.
727ed0f to
d9efde7
Compare
|
As discussed, I'll land this pre-review to unblock dependent work. I won't wait for Travis. I'll fix lint errors in master. |
I'll take a page out of @gaearon's playbook and list out my todo in an early PR as I'm implementing it:
Change side-effects to reverse order.